Add Classloader precheck to expensive matchers.#1283
Conversation
Benchmark results: ``` Benchmark Mode Cnt Score Error Units ClassRetransformingBenchmark.WithAgent.testTracedRetransform avgt 21.933 ms/op ClassRetransformingBenchmark.WithAgent.testUntracedRetransform avgt 6.171 ms/op ClassRetransformingBenchmark.WithAgentMaster.testTracedRetransform avgt 22.129 ms/op ClassRetransformingBenchmark.WithAgentMaster.testUntracedRetransform avgt 6.517 ms/op ClassRetransformingBenchmark.testTracedRetransform avgt 0.876 ms/op ClassRetransformingBenchmark.testUntracedRetransform avgt 0.867 ms/op ``` I also saw a small improvement in application startup time.
randomanderson
left a comment
randomanderson
left a comment
There was a problem hiding this comment.
👍
2 comments:
- It seems like
not(classLoaderHasNoResources(...))is the default so maybe the matcher itself should be inverted - I wish this could be added automagically at compile time when
implementsInterfaceis detected. I'll keep it in mind for the future 📓
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderMatcher() { | ||
| // Optimization for expensive typeMatcher. | ||
| return not(classLoaderHasNoResources("com/amazonaws/AmazonWebServiceRequest.class")); |
There was a problem hiding this comment.
The double negative is a bit hard to read. I think a utility method would help clarity greatly.
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderMatcher() { | ||
| // Optimization for expensive typeMatcher. | ||
| return not(classLoaderHasNoResources("org/hibernate/Session.class")); |
There was a problem hiding this comment.
One observation from the field injection optimization. Often our classLoaderMatchers are really more aim at matching a library or an API, but we don't really have that as a concept in the code.
I think this change also suggest that we should have some notion of a Library go which Instrumenters refer.
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderMatcher() { | ||
| return classLoaderHasNoResources("redis/clients/jedis/commands/ProtocolCommand.class"); | ||
| return classLoaderHasNoResources("redis/clients/jedis/commands/ProtocolCommand.class") |
There was a problem hiding this comment.
I think this speaks to the fact that we should make a classLoaderHasResources. Then this could be a single matcher.
There was a problem hiding this comment.
Yeah, it would be nice to have a comment here explaining what's going on
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
While I do have some quibbles with the details, overall, I like the change. It gives us a cheap way to effectively check whether or not an Instrumenter needs to be activated which is nice.
I'm wondering if we'd get a bit more by caching and/or introducing a first class of concept of a Library, so that we can further reduce the number of Matchers being checked.
I do have one theoretical concern about whether checking getResource is really a reliable way to check for existence of a class. I don't believe getResource traverses through parent loaders (maybe the Matcher accounts for that) and can be overridden separately findClass.
However, I think that complaint is somewhat fundamental in how ByteBuddy works already. I think that's an issue we need to revisit, but not necessarily as part of this PR.
|
I agree with the complaints about the double negative, though I was trying to get something out quickly. I think this offers a decent amount of improvement, so I'm going to merge as is, and we can plan for future iterations. |
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderMatcher() { | ||
| // Optimization for expensive typeMatcher. | ||
| return not(classLoaderHasNoResources("org/apache/http/nio/client/HttpAsyncClient.class")); |
There was a problem hiding this comment.
Double negatives are somewhat hard to read
4 participants
Benchmark results:
I also saw a small improvement in application startup time.